Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Draft: Try to use use rust-jaeger-python-client #13399

Closed

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Jul 26, 2022

Try to use use rust-jaeger-python-client

Dev notes

Installing rust-python-jaeger-reporter

I wired up the Complement tests to send OpenTelemetry traces to Jaeger but I'm not seeing all of the traces so I wanted to try rust-python-jaeger-reporter which is a faster more efficient Jaeger reporter that Erik wrote. Synapse will use this reporter if it's installed, see synapse/logging/opentracing.py#L256

Since that package only has wheels for many_linux, it isn't available for macOS and it tries to build manually but it doesn't have an sdist/pyproject.toml or whatever is necessary for that (discussed in #synapse-dev). But there was recently a PR to add it erikjohnston/rust-jaeger-python-client#5, but there hasn't been a release with it yet so we have to install it from repo,

poetry add git+https://github.com/erikjohnston/rust-jaeger-python-client.git

But the Docker images don't have a new enough version of Rust to build it manually:

#18 58.72 Building wheels for collected packages: thrift, rust-python-jaeger-reporter
#18 58.73   Building wheel for thrift (setup.py): started
#18 66.05   Building wheel for thrift (setup.py): finished with status 'done'
#18 66.05   Created wheel for thrift: filename=thrift-0.16.0-cp39-cp39-linux_x86_64.whl size=187043 sha256=7cc5bd6e095168a90e464110faa7a0cb260de2b5c159df70f9658b44c429373e
#18 66.05   Stored in directory: /root/.cache/pip/wheels/67/b1/a4/cf8b927fdbee8e55234f1f421b531685b960e5f23b6273cfcf
#18 66.07   Building wheel for rust-python-jaeger-reporter (pyproject.toml): started
#18 78.20   Building wheel for rust-python-jaeger-reporter (pyproject.toml): finished with status 'error'
#18 78.22   error: subprocess-exited-with-error
#18 78.22
#18 78.22   × Building wheel for rust-python-jaeger-reporter (pyproject.toml) did not run successfully.
#18 78.22   │ exit code: 1
#18 78.22   ╰─> [63 lines of output]
[...]
error: could not compile `parking_lot_core`.
error[E0658]: use of unstable library feature 'renamed_spin_loop'
[...]

We need the latest version of Rust since getting it from apt will give us 1.48.0 which is too old for the Rust renamed_spin_loop usage that the library uses.

RUN apt-get update -qq && apt-get install -yqq rustc
RUN apt-cache policy rustc
#11 [builder 3/8] RUN apt-cache policy rustc
#11 sha256:f16de148c0f960a46efaa25d0b01e2a17244e9754c0bd08b37aa55aac1c2230b
#11 0.455 rustc:
#11 0.455   Installed: 1.48.0+dfsg1-2
#11 0.455   Candidate: 1.48.0+dfsg1-2
#11 0.455   Version table:
#11 0.455  *** 1.48.0+dfsg1-2 100
#11 0.456         100 /var/lib/dpkg/status
#11 DONE 0.5s

So we can install the latest version of Rust this way:

# Install curl as a prerequisite
RUN  apt-get update -qq && apt-get install -yqq curl
# Install Rust and Cargo
RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | bash -s -- -y
# Add .cargo/bin to PATH
ENV PATH="/root/.cargo/bin:${PATH}"
# Check cargo is visible
RUN cargo --help

Then you can check the server logs for Using rust_python_jaeger_reporter library to know if it's successfully using it.

I got it installed and Synapse is using it but it logs an error about no active span even though you can see the opentracing.start_active_span call just above the spot it's complaining about

$ TEST_ONLY_SKIP_DEP_HASH_VERIFICATION=1 COMPLEMENT_ALWAYS_PRINT_SERVER_LOGS=1 COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh -run TestMessagesOverFederation -p 1
[...] 
_handle_new_device_update_async-0 - There was no active span when trying to log. Did you forget to start one or did a context slip?
synapse_main | Stack (most recent call last):
synapse_main |   File "/usr/local/lib/python3.9/threading.py", line 937, in _bootstrap
synapse_main |     self._bootstrap_inner()
synapse_main |   File "/usr/local/lib/python3.9/threading.py", line 980, in _bootstrap_inner
synapse_main |     self.run()
synapse_main |   File "/usr/local/lib/python3.9/threading.py", line 917, in run
synapse_main |     self._target(*self._args, **self._kwargs)
synapse_main |   File "/usr/local/lib/python3.9/site-packages/twisted/_threads/_threadworker.py", line 47, in work
synapse_main |     task()
synapse_main |   File "/usr/local/lib/python3.9/site-packages/twisted/_threads/_team.py", line 182, in doWork
synapse_main |     task()
synapse_main |   File "/usr/local/lib/python3.9/site-packages/twisted/python/threadpool.py", line 244, in inContext
synapse_main |     result = inContext.theWork()  # type: ignore[attr-defined]
synapse_main |   File "/usr/local/lib/python3.9/site-packages/twisted/python/threadpool.py", line 260, in <lambda>
synapse_main |     inContext.theWork = lambda: context.call(  # type: ignore[attr-defined]
synapse_main |   File "/usr/local/lib/python3.9/site-packages/twisted/python/context.py", line 117, in callWithContext
synapse_main |     return self.currentContext().callWithContext(ctx, func, *args, **kw)
synapse_main |   File "/usr/local/lib/python3.9/site-packages/twisted/python/context.py", line 82, in callWithContext
synapse_main |     return func(*args, **kw)
synapse_main |   File "/usr/local/lib/python3.9/site-packages/twisted/enterprise/adbapi.py", line 282, in _runWithConnection
synapse_main |     result = func(conn, *args, **kw)
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/storage/database.py", line 970, in inner_func
synapse_main |     return func(db_conn, *args, **kwargs)
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/storage/database.py", line 712, in new_transaction
synapse_main |     opentracing.log_kv({"message": "commit"})
synapse_main |   File "/usr/local/lib/python3.9/site-packages/synapse/logging/opentracing.py", line 390, in ensure_active_span_inner_2
synapse_main |     logger.error(

But with rust-python-jaeger-reporter, I'm not seeing any traces make their way back to Jaeger though yet.

Probably because rust-python-jaeger-reporter "is not configurable and is hardcoded to report to the local agent on localhost and the default port.". (need to configure to go from Docker container to the host host.docker.internal. With the normal jaeger-client-python you can use opentracing.jaeger_config.local_agent.reporting_host in Synapse to configure this.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

jupyter = ["ipywidgets (>=7.5.1,<8.0.0)"]

[[package]]
name = "rust-python-jaeger-reporter"
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I add only rust-python-jaeger-reporter without changing the rest of the poetry.lock file?

poetry add git+https://github.com/erikjohnston/rust-jaeger-python-client.git

Copy link
Contributor Author

@MadLittleMods MadLittleMods Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe poetry lock --no-update but that still seems to change many many things

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

poetry lock --no-update is what you want. You might need to update to poetry 1.1.14 and clear caches to workaround this.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DMRobertson, it seemed to change less stuff this time after clearing the cache but still looks dubious, b732dab

$ poetry --version
Poetry version 1.1.14
$ poetry cache list
pypi
$ poetry cache clear --all pypi
Delete 605 entries? (yes/no) [no] yes
$ poetry lock --no-update
Resolving dependencies... (125.3s)

Writing lock file

# Commited the result

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The closer I look at it, maybe it's just re-ordering some stuff which is fine 🤷

Copy link
Contributor

@DMRobertson DMRobertson Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this (re-ordering) earlier today on another branch. I don't have a good explanation either :(

@@ -182,6 +182,7 @@ hiredis = { version = "*", optional = true }
Pympler = { version = "*", optional = true }
parameterized = { version = ">=0.7.4", optional = true }
idna = { version = ">=2.5", optional = true }
rust-python-jaeger-reporter = {git = "https://github.com/erikjohnston/rust-jaeger-python-client.git"}
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rust-python-jaeger-reporter "is not configurable and is hardcoded to report to the local agent on localhost and the default port.". (need to configure to go from Docker container to the host host.docker.internal. With the normal jaeger-client-python you can use opentracing.jaeger_config.local_agent.reporting_host in Synapse to configure this.

(╯°□°)╯︵ ┻━┻ time to use OpenTelemetry stuff #11850

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> #13400

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated rust-python-jaeger-reporter to make the host and port configurable via erikjohnston/rust-jaeger-python-client#8

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants